Add Range-based Multipart download Subscriber for Pre-signed URLs #6331

jencymaryjoseph · 2025-08-11T22:33:09Z

Motivation and Context

This PR introduces support for multipart downloads using presigned URLs. Unlike the existing MultipartDownloaderSubscriber which relies on partNumber, this implementation uses HTTP range headers to achieve multipart functionality with pre-signed URLs.

Modifications

PresignedUrlMultipartDownloaderSubscriber: New reactive subscriber that handles size discovery through initial range requests, manages multipart state with thread-safe operations, and validates response consistency using ETag comparison.

Testing

PresignedUrlMultipartDownloaderSubscriberTckTest: Reactive Streams specification compliance test using SubscriberWhiteboxVerification with mocked S3AsyncClient for isolated testing.
PresignedUrlMultipartDownloaderSubscriberWiremockTest: WireMock tests covering multipart downloads with various part sizes, single-part downloads, and error scenarios.
PresignedUrlMultipartDownloadTestUtil: WireMock stubbing utilities for range request simulation and verification.

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

.../amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java

...ware/amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloadTestUtil.java

zoewangg · 2025-08-12T23:36:01Z

.../amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java

+
+    @Override
+    public void onSubscribe(Subscription s) {
+        synchronized (lock) {


Why do we need synchronized here?

ah right, this was defensive programming, we don't need synchronization here since Reactive Streams spec guarantees onSubscribe is called at most once per Subscriber.

zoewangg · 2025-08-12T23:41:35Z

.../amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java

+    private long parseContentRangeForTotalSize(String contentRange) {
+        Matcher matcher = CONTENT_RANGE_PATTERN.matcher(contentRange);
+        if (!matcher.matches()) {
+            throw new IllegalArgumentException("Invalid Content-Range header: " + contentRange);


~~Same here~~

Did we handle the exception elsewhere? because this may cause the request to get stuck, we probably wanted to make sure we handle it elegantly, cancelling subscription, failing the future exceptionally etc

zoewangg · 2025-08-12T23:42:12Z

.../amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java

+        }
+    }
+
+    private long parseContentRangeForTotalSize(String contentRange) {


Can we move this class to a util or something and add some unit tests?

zoewangg · 2025-08-12T23:44:17Z

.../amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java

+        future.complete(null);
+    }
+
+    public CompletableFuture<Void> future() {


Is this used anywhere?

future() method is used for testing to verify subscriber completion/error states

I see, can we add SdkTestInternalApi annotation?

zoewangg · 2025-08-12T23:45:22Z

...dk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriberWiremockTest.java

+import software.amazon.awssdk.services.s3.utils.AsyncResponseTransformerTestSupplier;
+
+@WireMockTest
+class PresignedUrlMultipartDownloaderSubscriberWiremockTest {


Instead of testing PresignedUrlMultipartDownloaderSubscriber, can we add some tests using S3AsyncClient directly? Example: https://github.com/aws/aws-sdk-java-v2/blob/master/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientPutObjectWiremockTest.java

zoewangg · 2025-08-13T00:22:37Z

.../amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java

+                    this.totalContentLength = parseContentRangeForTotalSize(response.contentRange());
+                    this.totalParts = calculateTotalParts(totalContentLength, configuredPartSizeInBytes);
+                    log.debug(() -> String.format("Total content length: %d, Total parts: %d", totalContentLength, totalParts));
+                } catch (Exception e) {


Using try catch as control flow it is a bit smell. Suggesting https://github.com/aws/aws-sdk-java-v2/blob/master/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/KnownContentLengthAsyncRequestBodySubscriber.java#L193

Refactored to use Optional<SdkClientException> validation pattern like KnownContentLengthAsyncRequestBodySubscriber instead of try-catch for control flow.

zoewangg · 2025-08-13T00:24:00Z

.../amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java

+                    return;
+                }
+            }
+            if (totalParts != null && totalParts > 1 && totalComplete < totalParts) {


Suggesting creating a method to make it more clear, maybe hasMoreParts()?

zoewangg · 2025-08-13T00:24:36Z

.../amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java

+        }
+    }
+
+    private void validateResponse(GetObjectResponse response) {


Can we add all validations specified in the transfer manager SEP?

sonarqubecloud · 2025-08-13T00:32:19Z

Quality Gate passed

Issues
38 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

zoewangg · 2025-08-13T00:27:53Z

.../amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java

+            if (eTag == null) {
+                this.eTag = response.eTag();
+                log.debug(() -> String.format("Multipart object ETag: %s", this.eTag));
+            } else if (response.eTag() != null && !eTag.equals(response.eTag())) {


Can we add a test case for this?

Removed ETag validation, now we capture the ETag from the first part and use it for If-Match header validation on subsequent parts which aligns with the SEP requirements.

zoewangg · 2025-08-13T00:29:32Z

.../amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java

+        return (int) Math.ceil((double) contentLength / partSize);
+    }
+
+    private PresignedUrlDownloadRequest createPartRequest(int partIndex) {


nit: suggesting createRangedGetRequest since this it not part request

zoewangg · 2025-08-13T00:31:10Z

...dk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriberWiremockTest.java

+                                     .endpointOverride(URI.create("http://localhost:" + wiremock.getHttpPort()))
+                                     .serviceConfiguration(S3Configuration.builder()
+                                                                          .pathStyleAccessEnabled(true)
+                                                                          .checksumValidationEnabled(false)


Why are we disabling checksum validation?

Range requests don't have checksums to validate, so this setting has no effect but false would be more explicit

It seems a bit odd though since it's enabled by default and we are testing with it off explicitly. Can we enable checksum mode by default (I think we should if we can)

zoewangg · 2025-08-13T00:37:08Z

.../amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java

+        int totalComplete = completedParts.get();
+        log.debug(() -> String.format("Completed part %d", totalComplete));
+
+        synchronized (lock) {


Do we need to lock the entire block?

We can reduce the sync lock scope and make totalContentLength, totalParts, and eTag volatile.

Copilot

Pull Request Overview

This PR introduces a new implementation for multipart downloads using presigned URLs with HTTP range headers instead of S3's native partNumber-based multipart API. The implementation uses reactive streams to handle concurrent part downloads while maintaining thread safety and proper response validation.

Adds PresignedUrlMultipartDownloaderSubscriber class for range-based multipart downloads with presigned URLs
Implements size discovery through initial range requests and ETag validation for response consistency
Provides comprehensive test coverage including TCK compliance tests and WireMock integration tests

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
PresignedUrlMultipartDownloaderSubscriber.java	Core subscriber implementation for presigned URL multipart downloads using HTTP range requests
PresignedUrlMultipartDownloaderSubscriberTckTest.java	Reactive Streams TCK compliance test with mocked S3AsyncClient
PresignedUrlMultipartDownloaderSubscriberWiremockTest.java	Integration tests using WireMock for various download scenarios
PresignedUrlMultipartDownloadTestUtil.java	Test utility class for WireMock stubbing and verification

Comments suppressed due to low confidence (2)

services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloadTestUtil.java:138

The XML tag '' appears to be incomplete or truncated. It should likely be '' to match the pattern used in line 125.

                .withBody("<e><Code>400</Code><Message>test error message</Message></e>")));

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.}

jencymaryjoseph requested a review from a team as a code owner August 11, 2025 22:33

davidh44 reviewed Aug 12, 2025

View reviewed changes

jencymaryjoseph requested a review from davidh44 August 12, 2025 22:59

davidh44 approved these changes Aug 12, 2025

View reviewed changes

zoewangg requested changes Aug 13, 2025

View reviewed changes

zoewangg reviewed Aug 13, 2025

View reviewed changes

zoewangg requested a review from Copilot August 13, 2025 00:37

Copilot AI reviewed Aug 13, 2025

View reviewed changes

jencymaryjoseph added 6 commits August 13, 2025 23:31

Range based multipart download subscriber

9ec0672

Refactor code and add tests

6afcafa

Fixed sonarcube issue

363accc

Address PR comments: use Long type, inline variables, allow empty files

f30096b

Refactored subscriber and tests

1704682

delete testutil

c12ea47

jencymaryjoseph force-pushed the feature/multipartSubscriber branch from e9d282d to c12ea47 Compare August 14, 2025 15:56

jencymaryjoseph requested a review from zoewangg August 14, 2025 15:58

Add Range-based Multipart download Subscriber for Pre-signed URLs #6331

Are you sure you want to change the base?

Add Range-based Multipart download Subscriber for Pre-signed URLs #6331

Uh oh!

Conversation

jencymaryjoseph commented Aug 11, 2025

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoewangg Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Aug 13, 2025

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

zoewangg Aug 12, 2025 •

edited

Loading